Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor router #79

Merged
merged 2 commits into from
Nov 12, 2024
Merged

Refactor router #79

merged 2 commits into from
Nov 12, 2024

Conversation

palagdan
Copy link
Collaborator

@palagdan palagdan commented Nov 5, 2024

Resolve #78

@palagdan
Copy link
Collaborator Author

palagdan commented Nov 5, 2024

@blcham

I updated react-router-dom to the latest version and renamed the App component to Router since that name is more semantically appropriate. I also moved the Router component from the components folder to the src directory because the components folder in a React application is meant for reusable components.

Additionally, I removed react-imported-component because it can be replaced by the Lazy and Suspense components from the React library itself. While react-imported-component is useful for splitting routes into chunks and loading components only when needed, I believe it's unnecessary for this app since it primarily benefits larger applications by preventing the initial load of too many components. In this case, it seems like overhead. Also this library is not used by so many people(3k downloads) and it is not supported as well.

I renamed Dagre to DagePage and created a workaround using a functional component Dagre because React v6 does not support withRouter; it only supports hooks. I can explain this further in our next meeting, but this change does not affect the application. The component will be rewritten after the refactoring is complete.

Overall, everything is functioning as before, but the code is now more readable.

@palagdan palagdan requested a review from blcham November 5, 2024 18:23
Copy link
Contributor

@blcham blcham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@blcham
Copy link
Contributor

blcham commented Nov 12, 2024

@palagdan after resolving conflicts you can merge

App component is refactored to Router component
react-router-dom bump to version 6
@palagdan palagdan merged commit 17a0ffe into master Nov 12, 2024
2 checks passed
@palagdan palagdan deleted the 78-refactor-router branch November 12, 2024 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor App component
2 participants